docs(adr): slice 3 design - security role model + moderation commands (ADR-0005)#6
Conversation
Deep design dive (docs/adr/0005-...): SecurityRole flags enum (full WheelMUD 12-value set), a hand-rolled Decorator pattern (RoleGuardedCommand/MuteGuardedCommand wrapping ICommand) instead of an attribute+reflection port or DecoWeaver (which only intercepts DI-container registration - sharp-mud's commands go through a custom ICommandRegistry, not IServiceCollection). ICommandRegistry.Register is replaced with two intentional entry points, RegisterOpen and RegisterWithRole, so no command can be registered without declaring its access level. Slice 3's command set: Boot/Mute/Unmute/Announce (MinorAdmin), Ban/Unban/RoleGrant/RoleRevoke (FullAdmin) - Unban added beyond WheelMUD's own list since an irreversible ban is an operability trap. SHARPMUD_INITIAL_ADMIN env var bootstraps the first FullAdmin, since RoleGrant itself requires FullAdmin to run. PLAN-0005 tracks execution; not yet implemented. Forward-references only added to SPEC.md/commands.md/accounts-auth.md per design.md - no code, no "current state" doc claims until tasks/implement.md actually builds this. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f705111a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Role accumulation: GrantRole(FullAdmin) now also grants MinorAdmin + Player (FullBuilder -> MinorBuilder), matching WheelMUD's actual behavior. Without this, a FullAdmin-only user failed every MinorAdmin-gated command since they're independent bits with no inherent relationship - the bootstrap admin couldn't have run day-to-day moderation commands. - Bootstrap timing: SHARPMUD_INITIAL_ADMIN is now checked in two places, not one - at boot (existing-world restart) AND at character-creation time in LoginFlow.MaybeCreateAsync (fresh-server case). A boot-time-only check was a no-op on a genuinely fresh server, since the target character doesn't exist yet at boot. - Repository access: the 6 admin commands needing offline target lookup (Mute/Unmute/Ban/Unban/RoleGrant/RoleRevoke) get IThingRepository via their own constructor, matching the existing ClassicCommands.RegisterAll pattern - not a CommandContext change. - Fixed ADR wording: MuteGuardedCommand checks the actor's own IsMuted (the speaker being blocked), not a "target's" - the plan already had this right, only the ADR prose was wrong. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08a35b9279
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The ADR cited WearableBehaviorConfiguration/Slot as an existing plain [Flags] bitmask precedent. EquipSlot (the actual type behind Slot) is a plain enum, not [Flags] - there is no existing [Flags] enum anywhere in this repo today. The "plain enum over OptimizedEnum" part of the citation was accurate and is kept; SecurityRole is correctly described as the first genuinely bitwise-combinable [Flags] enum in the codebase, not a second instance of an existing pattern. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1770819220
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
RevokeRole could clear a lower tier (e.g. MinorAdmin) while a higher tier that implies it (FullAdmin) remained set, breaking "FullAdmin implies MinorAdmin" for that user going forward - the same class of bug as the earlier grant-side gap, just on the way out instead of the way in. RevokeRole now rejects a revoke of a tier still implied by another currently-held role, naming the blocking tier, rather than silently applying an inconsistent state. Revoking the top tier itself (FullAdmin) still works normally and correctly leaves the tiers it implied (MinorAdmin/Player) intact - a demotion, not a cascading reset. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da4eab082c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Validating a role name against "is this a real SecurityRole name" would also accept All (every current AND future flag - not a real assignable tier, a severe over-grant) and None (a meaningless no-op sentinel), since both are literally named enum members. Restricted to an explicit allowlist of individually-grantable roles instead, rejecting All/None with a clear message. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aee0ab9292
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The ADR/plan only listed member names with no values. If implemented literally, C# auto-numbers unnumbered enum members sequentially (0, 1, 2, 3...) which are not distinct bits - Room would silently auto-number to 3, equal to Mobile | Item combined, breaking the bitwise-AND role check into granting unrelated permissions on overlapping bits. Added explicit 1 << n values to every member in both documents, and defined All as the union of the individual flags rather than a separate hardcoded value so it can't drift out of sync if a flag is added later. Added a dedicated "no two roles share a bit" regression test to the test plan. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff502de0c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
"Revoking MinorAdmin while FullAdmin is held" is a normal, directly-user-triggerable business-rule outcome (an admin ran rolerevoke), not a bug or an invariant violation - per coding-standards.md's Error Handling section, that must be a return value the caller checks, not a thrown exception. Changed RevokeRole's signature to string? (null on success, a message naming the blocking tier on failure), mirroring the existing MoveRequest.CancelReason nullable-reason idiom rather than a new pattern. RoleRevokeCommand checks the return value; never wraps the call in try/catch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3427721213
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
docs/deployment.md's Runtime Configuration table documents every HostOptions env var, and SHARPMUD_INITIAL_ADMIN was missing from both the plan's docs checklist and Critical files list - the only bootstrap path to a FullAdmin in a fresh deployment, easy to miss and leave a container with no discoverable way to administer it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bc37e103a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- HelpCommand would list every admin command's Verb/Aliases to every player (RoleGuardedCommand passes them through unchanged) - added a task to filter registry.Commands by the actor's roles, and a RequiredRole property on RoleGuardedCommand for it to check against. - "Online" was ambiguous for Boot/Announce, and the cited precedent (WhoCommand's iteration) doesn't actually do what the plan said - WhoCommand has no liveness filter at all, and since ADR-0004 that means Linkdead (disconnected-but-not-swept) players too. Made explicit: both need a ConnectionState == Playing check on top of the AllWithBehavior scan, not a blind copy of WhoCommand's loop. - RevokeRole's plan wording cited a nonexistent "MoveRequest.CancelReason" precedent - corrected to reference the actual type (UseExitEvent) and its real shape (a property set via .Cancel(), not a direct method return), while keeping the simpler string? return design. - No guard against self-lockout: an admin banning themselves has no in-game recovery, and a sole FullAdmin revoking their own tier has no path back without another FullAdmin present. Added explicit self-targeting rejection to BanCommand and RoleRevokeCommand. Added matching test-plan coverage for all four. Found via a full tasks/pr-review.md pass against this PR before further reviewer feedback, not from an external round. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Program.cs builds a fixed dictionary of only the three existing env vars before calling HostOptions.Parse(args, env) - not a full environment pass-through. Adding the parse logic to HostOptions.cs alone isn't enough; SHARPMUD_INITIAL_ADMIN needs its own dictionary entry or the real host never sees it even with the env var set. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37aae9dbc4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…l chain
- "Online" was ConnectionState == Playing alone, which is wrong:
ConnectionState is Ignore'd (not persisted), so it defaults to
Playing on any freshly-constructed PlayerBehavior - including a
player reloaded from the repository with no live session.
LoginFlow.FindAndAttachExistingAsync already registers such a
player into World before password verification even completes, so
this isn't hypothetical. Switched to the exact combined check
LoginFlow.LoginExistingAsync already established for this exact
purpose: ConnectionState == Playing && Session is {IsConnected:
true} - not a narrower, newly-invented check.
- The plan never specified how InitialAdminUsername actually reaches
LoginFlow.MaybeCreateAsync. Verified the real Telnet call chain
(HostRunner.RunTelnetAsync -> HandleConnectionAsync ->
LoginFlow.RunAsync) and added explicit tasks: TelnetHostContext
gains the field, LoginFlow.RunAsync/MaybeCreateAsync gain the
parameter, HostRunner and Program.cs thread it through.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4979668ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
BootCommand's DisconnectAsync ran in the admin's SessionLoop, not the target's, so explicitQuit never got set on the target's own loop and they could reconnect within the grace window as if merely Linkdead. Add PlayerBehavior.WasBooted (transient, MarkBooted()) as the signal that crosses the SessionLoop-instance boundary, plus test-plan and manual-verification coverage for the regression. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 767d3c5ccf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ADR-0005's Context inlined the full ActionSecurityAttribute/Actions.Admin source dive instead of recording it under docs/research/ per the documented design process. Added §11 to wheelmud-findings.md with the dive plus adopted/not-adopted notes, and pointed the ADR at it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac529bbd3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Bring in devops-templates workflows for automated dependency updates, grouped major/minor-patch dependabot PRs, and conventional-commit enforcement on PR titles. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…order Ban only set IsBanned/saved; SessionLoop never re-checks the flag mid-session, so an already-connected banned player kept issuing commands until an admin separately ran boot. BanCommand's task now mirrors BootCommand's live-disconnect handling. Also moved wheelmud-findings.md's new §11 above the closing Decisions section per docs/research/'s convention, folding its adopted/not-adopted summary into that section instead of appending after it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
Design-only (per
tasks/design.md— no application code). Deep design dive for Slice 3 of the WheelMUD reconciliation roadmap, conducted interactively:ADR-0005:SecurityRole[Flags]enum (full WheelMUD 12-value set), a hand-rolled Decorator pattern (RoleGuardedCommand/MuteGuardedCommand) rather than an attribute+reflection port of WheelMUD orLayeredCraft.DecoWeaver(ruled out — it only intercepts DI-container registration calls, and sharp-mud's commands go through a customICommandRegistry, notIServiceCollection, so adopting it would require an unrelated registration-pipeline rewrite first).ICommandRegistry.Register(ICommand)is replaced with two intentional entry points,RegisterOpen/RegisterWithRole— no command can be registered without declaring its access level.Boot/Mute/Unmute/Announce(MinorAdmin),Ban/Unban/RoleGrant/RoleRevoke(FullAdmin).Unbanadded beyond WheelMUD's own list (an irreversible ban is an operability trap). Deferred:Find/Locate/GoTo/Control/Clone/Spawn/Jail/Buff/Relinquish— each needs infra this slice doesn't build.SHARPMUD_INITIAL_ADMINenv var bootstraps the firstFullAdmin(solves the chicken-and-egg problem:RoleGrantitself requiresFullAdmin).PLAN-0005tracks execution,Status: Not Started.SPEC.md/docs/commands.md/docs/accounts-auth.md— no "current state" claims, since no code exists yet.Test plan
dotnet buildconfirmed clean).Next:
tasks/implement.mdbuilds this out.🤖 Generated with Claude Code